packages/plugins-shared/vite.config.mts (2)

8-8: Consider adding more DTS plugin options for better type generation

While the basic DTS plugin configuration works, consider adding these options for better TypeScript support:

• rollupTypes: true for better type bundling
• skipDiagnostics: false for stricter type checking

-  plugins: [dts({ insertTypesEntry: true, include: ['src'] })],
+  plugins: [dts({
+    insertTypesEntry: true,
+    include: ['src'],
+    rollupTypes: true,
+    skipDiagnostics: false
+  })],

---

16-17: Consider adding source maps for better debugging

For a shared library package, source maps would be valuable for debugging in development environments.

   formats: ['es', 'cjs'],
   fileName: format => `index.${format}.js`,
+ },
+ sourcemap: true,

packages/desktop-client/src/style/theme.tsx (2)

59-71: Performance note: Spread syntax used in reduce accumulators

While collecting plugin themes, repeated usage of spread syntax in “acc = { ...acc, ... }” inside a reduce can have O(n^2) complexity. If plugin lists become large, consider push or assignment-based merges to improve performance.

Below is an example of how you might refactor:

-acc = {
-  ...acc,
-  [theme]: {
-    name: theme,
-    colors: plugin.getThemeSchema(theme, false),
-  }
-};
+acc[theme] = {
+  name: theme,
+  colors: plugin.getThemeSchema(theme, false),
+};

---

72-84: Same performance note applies

The same pattern of object accumulator spread is repeated for the “themesDark”. Consider using property assignment to avoid repeated object copies if performance becomes a concern.

packages/desktop-client/src/components/settings/Themes.tsx (2)

60-78: Performance note on spread with reduce accumulators

Like in other files, spread syntax on each iteration can be inefficient at scale. Consider direct property assignment to “acc” object to avoid potential performance issues with large plugin arrays.

🧰 Tools

🪛 Biome (1.9.4)

[error] 69-69: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

---

80-98: Reusing accumulation pattern for 'themesDark'

Again, to maintain performance, avoid repeated full object copying for each iteration. Consider a direct assignment pattern.

🧰 Tools

🪛 Biome (1.9.4)

[error] 88-88: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

packages/desktop-client/src/components/ThemeSelector.tsx (4)

64-77: Accumulating plugin icons with repeated spread usage

Reusing spread syntax in a reduce accumulator can have performance downsides at scale. Consider direct property assignments to avoid O(n^2) complexity.

🧰 Tools

🪛 Biome (1.9.4)

[error] 70-70: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

---

79-92: Same pattern for 'pluginIconsDark'

Again, prefer direct assignments if performance concerns arise. Current approach is correct functionally, but not optimal in large plugin scenarios.

🧰 Tools

🪛 Biome (1.9.4)

[error] 84-84: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

---

93-111: Collecting plugin-provided themes (light, dark) via reduce

Similar performance reference: repeated object spreads in a loop can degrade performance. Library usage or direct assignments might be more efficient.

🧰 Tools

🪛 Biome (1.9.4)

[error] 101-101: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

---

133-134: Merging extended plugin themes

Same merge pattern as in other components. It’s functionally appropriate while having the same performance caution.

packages/plugins-shared/src/interfaces/actualPluginEntry.ts (1)

1-5: Well-designed plugin entry point type!

The ActualPluginEntry type follows a good pattern by injecting React as a parameter, which helps avoid potential React version conflicts between the host application and plugins. This approach ensures that plugins use the same React instance as the host application.

Consider documenting this pattern in the plugin development guide to help plugin authors understand why React is passed as a parameter and how to properly structure their plugins.

packages/plugins-shared/src/globals.d.ts (1)

1-6: Good approach for extending React's CSS types!

The extension of React's CSSProperties with Emotion's CSSObject is a clean way to enhance TypeScript's CSS type checking for your theming system. The ESLint disable comment is appropriately used and documented.

Consider adding a comment explaining why this extension is necessary for the theming system, to help future maintainers understand its purpose.

packages/plugins-shared/src/interfaces/actualPlugin.ts (3)

5-8: Add validation and documentation for plugin metadata

The required name and version fields lack format validation and documentation. Consider:

• Adding JSDoc comments describing expected formats
• Adding version format validation (e.g., semver)

Example documentation:

/**
 * Interface defining the contract for Actual Budget plugins
 */
export interface ActualPlugin {
  /** Unique identifier for the plugin. Should be kebab-case */
  name: string;
  /** Plugin version following semver (e.g., "1.0.0") */
  version: string;
  // ...
}

---

8-14: Document theme-related method contracts

The theme-related methods lack documentation about their expected behavior and return values. Add JSDoc comments describing:

• Expected theme name formats
• Icon requirements/constraints
• Schema structure expectations

---

15-22: Clean up or document future hooks

The commented-out hooks section suggests incomplete functionality. Either:

1. Remove the commented code until ready for implementation, or
2. Add TODO comments explaining the planned functionality and timeline

packages/loot-core/src/types/prefs.d.ts (1)

89-89: Document customTheme relationship with Theme type

The customTheme property lacks documentation explaining:

• Its relationship with the theme property
• When it's used vs. built-in themes
• Format expectations

packages/desktop-client/src/components/ActualPluginsProvider.tsx (2)

124-151: Improve plugin loading architecture

The current implementation has several architectural concerns:

1. No plugin validation/sandboxing
2. Basic error handling
3. No retry logic
4. No version management

Consider:

1. Implementing a plugin sandbox using Web Workers
2. Adding proper validation of plugin exports
3. Implementing retry logic with backoff
4. Adding version compatibility checks

---

83-122: Clean up or implement commented GitHub integration

The commented GitHub integration code should either be:

1. Implemented with proper error handling and security measures
2. Removed until ready for implementation
3. Documented with TODO explaining the implementation plan

packages/plugins-shared/src/interfaces/themeDefinition.ts (1)

1-196: Add JSDoc documentation and consider type safety improvements.

While the interface is well-structured, consider these enhancements:

1. Add JSDoc documentation to describe:
   • Expected color formats (hex, rgb, etc.)
   • Usage guidelines for different theme properties
   • Examples of valid values
2. Consider using more specific types for color values:

+/** Theme interface for customizing the application's appearance.
+ * @property pageBackground - Main background color (hex: #RRGGBB or rgb(r,g,b))
+ * @property pageText - Main text color
+ * ...
+ */
 export interface ThemeDefinition {
-  pageBackground?: string;
+  pageBackground?: `#${string}` | `rgb(${number},${number},${number})`;
   // Apply similar type constraints to other color properties

packages/loot-core/src/server/main.ts (2)

1335-1337: Add validation for customTheme value.

Consider adding validation for the customTheme value before saving it to ensure it contains valid theme data.

   if ('customTheme' in prefs) {
+    if (typeof prefs.customTheme !== 'string') {
+      throw new Error('customTheme must be a string');
+    }
     await asyncStorage.setItem('customTheme', prefs.customTheme);
   }

---

1383-1384: Consider extracting theme validation logic.

The theme validation logic could be more maintainable if extracted into a separate function. This would make it easier to add new themes and maintain the validation logic in one place.

+  const isValidTheme = (theme: string) => {
+    const validThemes = ['light', 'dark', 'auto', 'development', 'midnight'];
+    return validThemes.includes(theme) || Boolean(customTheme);
+  };
+
   return {
     floatingSidebar: floatingSidebar === 'true' ? true : false,
     maxMonths: stringToInteger(maxMonths || ''),
     documentDir: documentDir || getDefaultDocumentDir(),
     keyId: encryptKey && JSON.parse(encryptKey).id,
-    theme:
-      theme === 'light' ||
-      theme === 'dark' ||
-      theme === 'auto' ||
-      theme === 'development' ||
-      theme === 'midnight' ||
-      customTheme
-        ? theme
-        : 'auto',
+    theme: isValidTheme(theme) ? theme : 'auto',
     customTheme,
     preferredDarkTheme:
       preferredDarkTheme === 'dark' || preferredDarkTheme === 'midnight'

Also applies to: 1387-1387

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Search for any potential naming conflicts with 'Shared'
rg -l "(?:var|let|const|window\.)Shared\b" --type ts --type tsx

#!/bin/bash
# Search for any potential naming conflicts with 'Shared' in all files
rg -l "(?:var|let|const|window\.)Shared\b"

# Also check for any usage of 'Shared' in general to understand the context
rg "Shared" -C 2

#!/bin/bash
# Check if there are any direct imports of this module
rg -l "from ['\"].*globals['\"]" 

# Check if there are any references to the CSS properties extension
ast-grep --pattern 'interface $_ extends CSSProperties'

#!/bin/bash
# Check the content of the file importing globals
cat packages/loot-core/src/server/budget/util.ts

# Also check the content of globals.d.ts itself to understand what it declares
cat packages/plugins-shared/src/globals.d.ts

#!/bin/bash
# Description: Check what files would be ignored by the new pattern

# List all .js files in public directories
fd -g '**/public/*.js'

# Show the content of these files to verify they are indeed generated/build files
fd -g '**/public/*.js' -x head -n 5

#!/bin/bash
# Let's try a different approach to understand the codebase structure
# and potential impact of the ignore pattern

# First, let's see if there are any public directories
fd -t d 'public'

# Let's also check for any JavaScript files that might be affected
# by looking at the overall JS file structure
fd -e js

# Additionally, let's check if there's any documentation about
# the public directory usage in README or similar files
fd -g 'README*' -X grep -l 'public'

#!/bin/bash
# Let's examine the content of the only JS file found in public directory
# to verify if it's a generated/build file

head -n 10 "packages/desktop-client/public/index.es.js"

# Let's also check if there are any build configuration files
# that might indicate the purpose of the public directory
fd -g '*webpack*' -X grep -l 'public'